-
Notifications
You must be signed in to change notification settings - Fork 9
Refactor example app #139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor example app #139
Conversation
- Ignore `build` from docker compose. - Remove redundant lines.
Add Visual Studio Code configuration file for recommended extensions.
- Update NuGet packages to latest versions. - Add Grafana logging. - Remove redundant configuration file. - Add HttpClientFactory. - Use primary constructors. - Use async methods where available. - Use simple using statements. - Use structured logging. - Use collection expressions. - Remove unused fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the example ASP.NET Core app to use updated dependencies, modern C# patterns, and enhanced observability.
- Bumped NuGet package versions and removed unused config
- Added Grafana OpenTelemetry logging/metrics/tracing and HttpClientFactory
- Migrated controllers to primary constructors, async/await using, structured logging, and collection expressions
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
examples/net8.0/aspnetcore/aspnetcore.csproj | Updated package references to latest versions |
examples/net8.0/aspnetcore/appsettings.json | Added "System": "Warning" to log level settings |
examples/net8.0/aspnetcore/appsettings.Development.json | Removed redundant development config |
examples/net8.0/aspnetcore/Program.cs | Configured Grafana via OpenTelemetry and registered HttpClientFactory |
examples/net8.0/aspnetcore/Controllers/WeatherForecastController.cs | Switched to C# 12 collection expressions and removed unused logger |
examples/net8.0/aspnetcore/Controllers/RedisController.cs | Refactored to primary constructor DI, structured logging |
examples/net8.0/aspnetcore/Controllers/MsSqlController.cs | Migrated to async/await with await using and DI for SqlConnection |
examples/net8.0/aspnetcore/Controllers/HttpClientController.cs | Injected HttpClient , simplified calls and disposal |
.vscode/extensions.json | Added recommended VS Code extensions |
Comments suppressed due to low confidence (4)
examples/net8.0/aspnetcore/Controllers/RedisController.cs:19
- The
LeftPush
method no longer accepts a request body but still checksModelState
and pushes a hard-coded value. Reintroduce a[FromBody]
parameter or remove theModelState
check to align behavior with its signature.
public async Task<ActionResult<string>> LeftPush()
examples/net8.0/aspnetcore/Controllers/HttpClientController.cs:15
- The action is declared to return
IEnumerable<string>
but returns a single string fromGetStringAsync
. Change the return type toActionResult<string>
or return a collection.
public async Task<ActionResult<IEnumerable<string>>> Get()
examples/net8.0/aspnetcore/Program.cs:27
- [nitpick] Embedding the database connection string in code makes rotation and environment configuration harder. Consider retrieving it from configuration or a secret store.
var connectionString = "Server=mssql,1433;Database=master;User=sa;Password=Password12345%%;Encrypt=False;TrustServerCertificate=True";
examples/net8.0/aspnetcore/Controllers/HttpClientController.cs:15
- [nitpick] The new
Get
endpoint logic (using injectedHttpClient
) isn’t covered by any unit or integration tests. Add tests to verify response handling and error paths.
public async Task<ActionResult<IEnumerable<string>>> Get()
Cherry-pick changes from #137 to make that PR more focussed on fixing OATS.
build
from docker compose.